Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[13.0][ADD] product_abc_classification #623

Merged

Conversation

MiquelRForgeFlow
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow commented May 28, 2020

Product ABC Classification

This modules includes the ABC analysis (or ABC classification), which is
used by inventory management teams to help identify the most important
products in their portfolio and ensure they prioritize managing them above
those less valuable.

Managers will create a profile with several levels (percentages) and then the
profiled products will automatically get a corresponding level using the
ABC analysis.

Copy link
Contributor

@emagdalenaC2i emagdalenaC2i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Functional review. Looks really cool. Just a few comments

product_abc_classification/readme/USAGE.rst Outdated Show resolved Hide resolved
product_abc_classification/readme/USAGE.rst Outdated Show resolved Hide resolved
product_abc_classification/readme/USAGE.rst Outdated Show resolved Hide resolved
product_abc_classification/views/xyz_analysis_view.xml Outdated Show resolved Hide resolved
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch from 899ba4a to beb4b78 Compare May 28, 2020 12:39
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch 2 times, most recently from 33e1c20 to b3b38a5 Compare May 28, 2020 12:46
Copy link
Contributor

@emagdalenaC2i emagdalenaC2i left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the percentaje be unique?
See
Percentaje unique

@emagdalenaC2i
Copy link
Contributor

And please, could you add some tests?

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch 2 times, most recently from c12a750 to 977a934 Compare May 28, 2020 18:15
@emagdalenaC2i
Copy link
Contributor

image

@emagdalenaC2i
Copy link
Contributor

Why to use a selection in "Data source" and "Value" if there are just one single option to choose?

@emagdalenaC2i
Copy link
Contributor

Percentajes

@MiquelRForgeFlow
Copy link
Contributor Author

@emagdalenaC2i The screen about level should be unique its intended, You have repeated the number 40. And the selections are also intended, because maybe in the future it's extended.

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch from 977a934 to d2cafcb Compare May 29, 2020 08:11
@emagdalenaC2i
Copy link
Contributor

@emagdalenaC2i The screen about level should be unique its intended, You have repeated the number 40. And the selections are also intended, because maybe in the future it's extended.

So, I can't set an ABC setting of 20%-40%-40% ?

@MiquelRForgeFlow
Copy link
Contributor Author

So, I can't set an ABC setting of 20%-40%-40% ?

No, because it doesn't make sense. You can see explained here: https://www.eazystock.com/blog/how-to-use-abc-analysis-for-inventory-management/

@emagdalenaC2i
Copy link
Contributor

This module is a great idea, but you need to rethink the UI

@emagdalenaC2i
Copy link
Contributor

Ok, but make it sense to use a A-20% B-20% and C-60% settings?

@MiquelRForgeFlow
Copy link
Contributor Author

MiquelRForgeFlow commented May 29, 2020

Ok, but make it sense to use a A-20% B-20% and C-60% settings?

No. The percentages have to be unique.

@emagdalenaC2i
Copy link
Contributor

I think you could use a method like in the Payment terms
image

@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch 2 times, most recently from ce3b927 to a6690dc Compare May 29, 2020 14:16
@MiquelRForgeFlow MiquelRForgeFlow force-pushed the 13.0-add-product_ABC_classification branch from a6690dc to 7bf0c2e Compare May 29, 2020 14:20
@MiquelRForgeFlow
Copy link
Contributor Author

@JordiBForgeFlow done

@JordiBForgeFlow
Copy link
Sponsor Member

cc @LoisRForgeFlow @NuriaMForgeFlow @didierdonze

Copy link
Contributor

@legalsylvain legalsylvain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some remarks inline. Thanks for this new module.

class ProductTemplate(models.Model):
_inherit = "product.template"

abc_classification_profile_id = fields.Many2one(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi.

I don't understand why the abc_classification_profile_id is in the variant level, and not in the template level. Does it make sense to set a profile 1 for a variant, and a profile 2 to another variant of the same template ?

Propose to move the profile in the template level. (and just set a related in the variant level).

What do you think ?

Also, it could be great to have an api@depends("categ_id") to set the correct profile automatically when creating a new product, linked to a product_category with a profile defined.

Copy link
Contributor Author

@MiquelRForgeFlow MiquelRForgeFlow Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legalsylvain Sorry, I didn't have time to check this comment. Well, now if you think the code can be improved, then you can propose a PR for this version or do the refactor in the next one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arf... this require to write migration script to change the model.

I'll see.

But well, it doesn't answer to my question : Does it make sense to set a profile 1 for a variant, and a profile 2 to another variant of the same template ?

Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@legalsylvain From my POV it makes more sense in the variant. You can have a variant that is more successful in the market than other, and therefore you classify one as A and other as B.

My 2 cents.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure you get it.
Of course the level a/b/c should be in the variant.
But the perfil is common for all the variant of a product template, by design.

Concrete counter exemple welcome.

@dreispt
Copy link
Sponsor Member

dreispt commented Dec 2, 2021

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 13.0-ocabot-merge-pr-623-by-dreispt-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 6427513 into OCA:13.0 Dec 2, 2021
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at 8de111d. Thanks a lot for contributing to OCA. ❤️

@MiquelRForgeFlow MiquelRForgeFlow deleted the 13.0-add-product_ABC_classification branch December 2, 2021 17:26
DavidJForgeFlow pushed a commit to ForgeFlow/product-attribute that referenced this pull request Jul 26, 2022
Signed-off-by sbidoul
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants